#211 Namespace validation #215
Conversation
|
|
||
| do { | ||
| agreementKey = try kms.performKeyAgreement(selfPublicKey: selfPublicKey, peerPublicKey: proposal.proposer.publicKey) | ||
| try Namespace.validate(proposedNamespaces) |
There was a problem hiding this comment.
shouldn't proposedNamespaces be already validated when received proposal?
There was a problem hiding this comment.
Actually no, but it should. I'm changing it
| try Namespace.validate(sessionNamespaces) | ||
| try Namespace.validateApproved(sessionNamespaces, against: proposedNamespaces) |
There was a problem hiding this comment.
I think those two validations should not be in do{}catch{} block but rather throw an error so user can see that minimal requirement(or other requirement) has not been met.
I am also not sure if it make sense to respond with error for them in catch{} block
that is not valid error for a peer.
There was a problem hiding this comment.
I agree with the validation error catch, it has to be thrown. I just used it as a temporary solution to make the validation start working. An issue can be created for that to improve error reading.
The catch respond error already existed before, but only when the key agreement failed. Should this be changed also?
| @@ -209,6 +212,7 @@ final class PairingEngine { | |||
| try? pairing.updateExpiry() | |||
| } | |||
|
|
|||
| onApprovalResponse?(proposal) // FIXME: This proposal needs to be stored on creation and retrieved from cache here. Don't use proposal from response. | |||
There was a problem hiding this comment.
This proposal needs to be stored on creation and retrieved from cache here. Don't use proposal from response.
actually that proposal is from cache(jsonRpcHistory record) not from response, response do not contain proposal.
| pairingEngine.onProposeResponse = { [unowned self] sessionTopic in | ||
| sessionEngine.setSubscription(topic: sessionTopic) | ||
| } | ||
| pairingEngine.onApprovalResponse = { [unowned self] proposal in | ||
| sessionEngine.settlingProposal = proposal | ||
| } |
There was a problem hiding this comment.
do you need that extra callback onApprovalResponse ? onProposeResponse is executed in the same function.
would it work to have two arguments(if needed) in on callback?
There was a problem hiding this comment.
Yes, changed the callback to 2 args
| @@ -12,6 +12,8 @@ final class SessionEngine { | |||
| var onSessionDelete: ((String, SessionType.Reason)->())? | |||
| var onEventReceived: ((String, Session.Event, Blockchain?)->())? | |||
|
|
|||
| var settlingProposal: SessionProposal? | |||
There was a problem hiding this comment.
could it be overriden?
could some runtime key value storage be safer?
| private func onSessionSettle(payload: WCRequestSubscriptionPayload, settleParams: SessionType.SettleParams) { | ||
| logger.debug("Did receive session settle request") | ||
| guard let proposedNamespaces = settlingProposal?.requiredNamespaces else { |
There was a problem hiding this comment.
- you set
settlingProposalonApprovalResponseso it happens onresponder'sengine as responder calls "approve" - you retrieve that
settlingProposalonSessionSettleso it happens onproposer'sengine as responder calls "settle"
🤔 am I correct? will it work?
There was a problem hiding this comment.
splitting engines/methods for controller/non-controller later will really help.
There was a problem hiding this comment.
Actually they both happen on proposer's side, so it will work. The thing is that they happen on different engines. There may be a better solution to this.
Adds validation functions to new namespaces, applies planned test cases.
Integrates validation on proposal-approval flow.